lgj-abi: hop gathers instead of sweeping — the predicted crossover does not exist - #40
Merged
Conversation
…es not exist Closes ISS-LGJ-HOP-SWEEPS-FULL-POPULATION. The issue named a gather as the next rung and said its merit depended on a DENSITY CROSSOVER -- sparse frontiers favouring a gather, dense ones favouring the sweep's sequential vectorised access -- and that this had to be measured rather than assumed. Measured. THERE IS NO CROSSOVER. `examples/hop_gather_vs_sweep.rs` runs both shapes over five populations (1_024 ... 262_144) x twelve frontier densities, asserting BYTE-IDENTICAL output at every one of the 60 configurations plus an anti-vacuity guard against an empty hop (which would let both shapes "agree" on nothing and time two empty loops). Gather wins all 60: n_rows 0.01% 1% 25% 100% 1_024 361x 113x 7.3x 1.73x 4_096 1_295x 143x 5.9x 1.75x 65_536 2_666x 117x 2.5x 1.78x 262_144 2_754x 90x 2.6x 1.73x WHY IT HOLDS EVEN AT 100% DENSITY -- the part the prediction got wrong. A sweep MATERIALISES an n-element per-row intermediate that each row reads exactly once, so its cost is never amortised. At full density it does everything the gather does PLUS allocate, zero, write and re-read n u32s. It is strictly more work at EVERY density, not merely at sparse ones. The prediction reasoned about access PATTERN and missed that one shape simply does MORE. Consequence for the design, and it is the better outcome: NO THRESHOLD, NO DISPATCH, NO HEURISTIC GATE. A real crossover would have required one, with the two-sided evidence such a gate demands. Its absence makes the change unconditional. A SCALAR GATHER BEATS A VECTORISED ndarray::simd SWEEP. Same lesson the object-model ladder taught: the win is in not doing the work, not in the vector width. No SIMD primitive is called in the hop now and none is missing -- `simd_rowstore_facet_match` remains the kernel behind `lgj_row_facet_match` and `simd_rowstore_classid_mask` behind `lgj_op_eq_classid`, so neither is orphaned (checked, not assumed). END TO END through Component G, the independent instrument: native_hop 1%/4096 1%/65536 25%/4096 25%/65536 original (32x) 479.0 24798.3 521.2 23633.9 us/op one-pass (#39) 374.7 7120.3 375.8 8076.9 gather 1.9 34.4 61.0 3534.1 vs original 246x 720x 8.5x 6.7x The ordering the component was built to test has INVERTED: native is now fastest at every configuration, 2.0x-13x ahead of the best scalar arm, having begun this arc slowest at every configuration by 2.6x-165x. The two instruments are independent and agree: the Rust probe measures the sweep at 6754-8118 us at 1%/65536 against JMH's 7120 us. HONESTY on the JMH re-run: it was noisier than the previous one -- `classidScan` at 1%/4096 reports 25.6 +/- 61.9 us, an error bar larger than the score -- so the SCALAR absolutes in that table are weak. Native's own errors are tight (1.9 +/- 0.3, 34.4 +/- 8.1) and a 720x change is far outside any noise this container produces. ALSO FIXES A DOC I STALED ONE PR AGO. `lgj_hop`'s "Kernel composition" block still described the pre-#39 design (one full-width classid sweep PER FACET, a scratch buffer "REUSED across every participating facet"). #39 changed the code and left the prose. Rewritten to describe the gather, with both superseded shapes named so the arc is legible. The overflow guard is KEPT as an explicit check rather than dropped or hidden behind an `_n` binding: the gather no longer needs `n`, but row indices are still cast to usize, so a store whose row count does not fit must still be refused there. STILL OPEN, named rather than hidden: the one shape that could favour a precomputed per-row mask is REUSE -- memoising it across many hops on the same (store, classid). That is a caching design with its own invalidation questions and is deliberately not this function's. Gates: lgj-abi 134/134; AllTests 304, GraphHopTest 66 (incl. G3 at its unchanged allocation floor), TradesParity 12, TradesAllocation 3, BricksAuth 62 = 447 Java checks; clippy -D warnings and fmt clean. No ABI change, no signature change, no minor bump -- an internal shape change behind an unchanged contract. Board artifacts same commit: ISSUES resolved with the corrected mechanism and the remaining reuse question, RESULTS.md § G carries both tables and the noise caveat, raw probe output banked at .claude/board/hop-gather-vs-sweep-crossover.txt.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3e476914-6007-4c4e-a00b-01330ba36499) |
AdaWorldAPI
marked this pull request as ready for review
August 27, 2026 13:37
AdaWorldAPI
pushed a commit
that referenced
this pull request
Aug 27, 2026
The "Kernel composition" paragraph still described the single-predicate shape: one classid match per facet, with the structured-edge gate absent because it was an `if` in the walk rather than an operand. R1 makes selection `src AND class_f AND struct_f`, so §13 now names both predicates, the one primitive that produces both (same call, offsets +0 and +12 into the facet), and the reason the scatter alone stays scalar — the destination is decoded from the selected row's payload, which makes it the operand of a permutation rather than a decision about membership. Caught by re-reading the diff adversarially rather than by a gate. Leaving prose behind a changed function is exactly what #39 did and #40 had to fix one PR later; no reason to make the same reviewer spend that cycle twice. No ABI change: no symbol, no signature, no minor bump. Gates re-run unchanged — 134/134, clippy -D warnings, fmt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DCfrD5y19cvFc4AoyydXYv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes
ISS-LGJ-HOP-SWEEPS-FULL-POPULATION. The issue named a gather as the next rung and said its merit depended on a density crossover — sparse frontiers favouring a gather, dense ones favouring the sweep's sequential vectorised access — and that this had to be measured rather than assumed.Measured. There is no crossover.
60 configurations, gather wins all 60
examples/hop_gather_vs_sweep.rsruns both shapes over five populations × twelve densities, asserting byte-identical output at every configuration plus an anti-vacuity guard against an empty hop (which would let both shapes "agree" on nothing and time two empty loops).Why it holds even at 100 % density — the part the prediction got wrong
A sweep materialises an
n-element per-row intermediate that each row reads exactly once, so its cost is never amortised. At full density it does everything the gather does plus allocate, zero, write and re-readnu32s. It is strictly more work at every density, not merely at sparse ones.The prediction reasoned about access pattern and missed that one shape simply does more.
The better outcome: no gate at all
A real crossover would have required a threshold, a dispatch, and the two-sided evidence a heuristic gate demands. Its absence makes the change unconditional — simpler than what was planned for.
A scalar gather beats a vectorised
ndarray::simdsweepSame lesson the object-model ladder taught: the win is in not doing the work, not in the vector width. No SIMD primitive is called in the hop now and none is missing —
simd_rowstore_facet_matchremains the kernel behindlgj_row_facet_match,simd_rowstore_classid_maskbehindlgj_op_eq_classid. Neither is orphaned (checked, not assumed).End to end through Component G
native_hopThe ordering the component was built to test has inverted. Native is now fastest at every configuration, 2.0×–13× ahead of the best scalar arm — having begun this arc slowest at every configuration by 2.6×–165×.
The two instruments are independent and agree: the Rust probe measures the sweep at 6 754–8 118 µs at 1 %/65 536 against JMH's 7 120 µs.
Honesty on the JMH re-run: it was noisier than the previous one —
classidScanat 1 %/4096 reports 25.6 ± 61.9 µs, an error bar larger than the score — so the scalar absolutes in that table are weak. Native's own errors are tight (1.9 ± 0.3, 34.4 ± 8.1), and a 720× change is far outside any noise this container produces.Also fixes a doc I staled one PR ago
lgj_hop's "Kernel composition" block still described the pre-#39 design — one full-width classid sweep per facet, a scratch buffer "REUSED across every participating facet". #39 changed the code and left the prose. Rewritten to describe the gather, with both superseded shapes named so the arc stays legible.The overflow guard is kept as an explicit check rather than dropped or hidden behind an
_nbinding: the gather no longer needsn, but row indices are still cast tousize, so a store whose row count doesn't fit must still be refused there.Still open, named rather than hidden
The one shape that could favour a precomputed per-row mask is reuse — memoising it across many hops on the same
(store, classid). That's a caching design with its own invalidation questions, and deliberately not this function's.Gates
lgj-abi 134/134; AllTests 304, GraphHopTest 66 (incl. G3 at its unchanged allocation floor), TradesParity 12, TradesAllocation 3, BricksAuth 62 = 447 Java checks;
clippy -D warningsandfmtclean.No ABI change, no signature change, no minor bump — an internal shape change behind an unchanged contract.
Raw probe output banked at
.claude/board/hop-gather-vs-sweep-crossover.txt.Generated by Claude Code